-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restores test and refactors component and test for layout-super-navigation-header #3939
Restores test and refactors component and test for layout-super-navigation-header #3939
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on adding this functionality back in to the super navigation menu.
The code itself looks good to me, just made some small suggestions around the naming of things.
app/assets/javascripts/govuk_publishing_components/components/layout-super-navigation-header.js
Outdated
Show resolved
Hide resolved
app/assets/javascripts/govuk_publishing_components/components/layout-super-navigation-header.js
Outdated
Show resolved
Hide resolved
spec/javascripts/components/layout-super-navigation-header-spec.js
Outdated
Show resolved
Hide resolved
spec/javascripts/components/layout-super-navigation-header-spec.js
Outdated
Show resolved
Hide resolved
0baa569
to
a799630
Compare
Thanks @MartinJJones - I've made those changes and updated the CHANGELOG for this PR. (I'll squash the commits if it's all OK). Could you have another look over it? Cheers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, changes look good to me 👍
89957f8
to
fe7c847
Compare
What
These changes restores a test that was temporarily removed and refactors both that and the component it relates to.
(Trello ticket: Refactor and improve recent change to layout-super-navigation-header component)
Why
When we made the changes in PR#3918 (Remove "Popular" links from super navigation header) we lost a recently added accessibility improvement, specifically to close the search menu when a keyboard user tabs out of that element. This was because this addition relied on the presence of links that were removed in the previous PR. We decided at the time to go ahead with that change and return to this to restore the lost behaviour.
This restores that by refactoring the original method dealing with that so that it doesn't rely on the presence of links but broadens the selector to include other tab-able elements. It also makes a small change to the original code for the search menu to make both more consistent with each other.